Not to consume buf when decoding non_huff header name(which consumes the buf) succeeded but header value failed (DecoderError::NeedMore) #589
Conversation
|
Anyone？ |
|
@hikaricai it would probably be easier to review this if we had a test that failed without this change. At a quick glance, this is going to incur an additional (unnecessary?) allocation on each call. |
|
@olix0r thanks, I will write a test case. It seems that the "fn consum" is used only if the decoding is success, but the "fn decode_string" may only be part of decoding. For example, decoding both header name and header value. I met decoding failure when receiving continuation header which just spilt header name and header value. |
Decoding error when processing continuation header which contains normal header name at boundary
|
I have just updated the commit, could anyone helps to review? |
|
Not sure if @carllerche has bandwidth to review, but I think he knows this code best. Also I think @seanmonstar and @nox have worked on this module ~recently. If no one's available, I'll try to take a look at this but it will take me some time to load up context. |
hikaricai
changed the title
hikaricai
changed the title
|
Could anyone helps to review? I need this patch to fix bug in my project. |
|
Sorry about that, I'll take a look now. |
src/hpack/decoder.rs
Outdated
| fn try_decode_string( | ||
| &mut self, | ||
| buf: &mut Cursor<&mut BytesMut>, | ||
| ) -> Result<((usize, usize), Option<Bytes>), DecoderError> { |
There was a problem hiding this comment.
It's hard to know what these values are supposed to be. What if instead of returning a tuple, it return some struct DecodedString { .. } that named the usizes and Option<Bytes>?
There was a problem hiding this comment.
Thank you for reply.
I am using tonic as grpc client and would receive a large HEADERS frame(grpc-status-details-bin) whose name is non_huff format and value is partial from C++ server. When decoding this header, the name is consumed because it is not huff, and then the value decoding is failed for DecoderError::NeedMore, and then the decode function returns with error but the buf is consumed.
I see that the CONTINUATION header currently tested in CI but both name and value are huff_encoded.
My first patch is simply replacing consume() with buf copy, and @olix0r pointed that the copy can be optimized. So i tried to save the decoding context and consume the buf only if both header name and header buf is successfully decoded.
I will update my patch to easily show the meaning of those values.
|
@seanmonstar Is the new commit clearly enough? The error situation is shown in the new test case |
Decoding error when processing continuation frames